Fix: ViewType gc on huge batch would produce bad output #8694
Conversation
| .map(|i| unsafe { self.copy_view_to_buffer(i, &mut data_buf) }) | ||
| .collect(); | ||
| for view in self.views() { | ||
| let len = *view as u32; |
There was a problem hiding this comment.
This part is so slow, but it's right, I can make it faster(by handling the numbers via grouping or batching) if required
There was a problem hiding this comment.
I am not sure how you would make this much faster - I think the code needs to find the locations to split in any event
There was a problem hiding this comment.
Even if the buffer size is greater than i32::MAX, it's possible that a single buffer is much smaller than i32::MAX, so this can find batch-by-batch, rather than just adding small buffer one-by-one?
There was a problem hiding this comment.
I see -- you are saying you could potentially optimize the input by looking at each input buffer or something and gcing it individually or something.
That would be interesting, though it would probably take a lot of care to make it fast.
| } | ||
|
|
||
| #[test] | ||
| fn test_gc_huge_array() { |
There was a problem hiding this comment.
This test requires about 5GiB memory, it's huge, I don't know would it affect the testing on some machines
There was a problem hiding this comment.
Previous code would meet bug only when buffer greater than 4GiB, the current code can be tested when > 2GiB. Personally I think leave 2GiB for test is ok but 4GiB is also ok to me, decide on reviewer's idea.
There was a problem hiding this comment.
I ran this test on my mac M3 and it takes 1.5 seconds so I think it is ok
running 1 test
test array::byte_view_array::tests::test_gc_huge_array ... ok
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 578 filtered out; finished in 1.58s
I also verified that without the code in this PR, the test fails like:
---- array::byte_view_array::tests::test_gc_huge_array stdout ----
thread 'array::byte_view_array::tests::test_gc_huge_array' panicked at arrow-array/src/array/byte_view_array.rs:1444:9:
assertion `left != right` failed: gc with huge buffer should not consolidate data into a single buffer
left: 1
right: 1
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
There was a problem hiding this comment.
I have removed the single buffer limitation :-(
There was a problem hiding this comment.
Sorry -- to be clear I was confirming that the test covers the code added in this PR . I think the test is good
|
@alamb Besides, I meet this bug when I have 4GiB StringViewArray, arrow-rs regard offset as u32, however, in arrow standard, this uses i32. So I limit it to 2GiB There're other places uses |
mapleFU
changed the title
| }; | ||
| vec![gc_copy_group] | ||
| }; | ||
| assert!(gc_copy_groups.len() <= i32::MAX as usize); |
There was a problem hiding this comment.
This assertion can be removed, I just ensure it would pass
There was a problem hiding this comment.
May be change to assert debug here.
There was a problem hiding this comment.
Thank you @mapleFU -- this is a good find. I left some comments, let me know what you think
cc @zhuqi-lucas perhaps you have some thoughts
| } | ||
|
|
||
| #[test] | ||
| fn test_gc_huge_array() { |
There was a problem hiding this comment.
I ran this test on my mac M3 and it takes 1.5 seconds so I think it is ok
running 1 test
test array::byte_view_array::tests::test_gc_huge_array ... ok
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 578 filtered out; finished in 1.58s
I also verified that without the code in this PR, the test fails like:
---- array::byte_view_array::tests::test_gc_huge_array stdout ----
thread 'array::byte_view_array::tests::test_gc_huge_array' panicked at arrow-array/src/array/byte_view_array.rs:1444:9:
assertion `left != right` failed: gc with huge buffer should not consolidate data into a single buffer
left: 1
right: 1
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
| total_buffer_bytes: total_large, | ||
| total_len: len, | ||
| }; | ||
| vec![gc_copy_group] |
There was a problem hiding this comment.
I know it is probably a small thing, but could you please avoid this new allocation for the common case where there is a single buffer? Perhaps by creating the data bufs directly via a function call rather than a loop over the array
There was a problem hiding this comment.
This makes sense, a fixed-sized slice on stack might be better in this scenerio
| .map(|i| unsafe { self.copy_view_to_buffer(i, &mut data_buf) }) | ||
| .collect(); | ||
| for view in self.views() { | ||
| let len = *view as u32; |
There was a problem hiding this comment.
I am not sure how you would make this much faster - I think the code needs to find the locations to split in any event
|
🤖 |
|
The MIRI test is probably failing due to the massive memory use in https://github.com/apache/arrow-rs/actions/runs/18818674867/job/53690752815?pr=8694 I suggest we don't run that test under miri by disabling it, with something like #[cfg_attr(miri, ignore)] // Takes too longFor example |
|
🤖: Benchmark completed Details
|
| }; | ||
| vec![gc_copy_group] | ||
| }; | ||
| assert!(gc_copy_groups.len() <= i32::MAX as usize); |
There was a problem hiding this comment.
May be change to assert debug here.
| } | ||
| &groups | ||
| } else { | ||
| one_group.as_slice() |
There was a problem hiding this comment.
I've add a one_group for gc-copy-group in stack, hopefully this avoids allocation
let one_group = [GcCopyGroup {
total_buffer_bytes: total_large,
total_len: len,
}];
|
@alamb I've ran the new code on MacOS M4 Pro. Don't know whether the performance comes from unstable environment or other This patch: main: |
There was a problem hiding this comment.
Thank you @mapleFU - I went through this again and it looks good to me.
I would like to run the benchmarks one more time before merging this (I have queued them up on my machine so hopefully they'll post to this PR in a few hours)
| } | ||
|
|
||
| #[test] | ||
| fn test_gc_huge_array() { |
There was a problem hiding this comment.
Sorry -- to be clear I was confirming that the test covers the code added in this PR . I think the test is good
| .map(|i| unsafe { self.copy_view_to_buffer(i, &mut data_buf) }) | ||
| .collect(); | ||
| for view in self.views() { | ||
| let len = *view as u32; |
There was a problem hiding this comment.
I see -- you are saying you could potentially optimize the input by looking at each input buffer or something and gcing it individually or something.
That would be interesting, though it would probably take a lot of care to make it fast.
|
🤖 |
|
🤖: Benchmark completed Details
|
| if len > MAX_INLINE_VIEW_LEN { | ||
| if current_length + len > i32::MAX as u32 { | ||
| // Start a new group | ||
| groups.push(GcCopyGroup { |
There was a problem hiding this comment.
I think you can preallocate groups (with_capacity) or use .collect
| for view_idx in current_view_idx..current_view_idx + gc_copy_group.total_len { | ||
| let view = | ||
| unsafe { self.copy_view_to_buffer(view_idx, group_idx as i32, &mut data_buf) }; | ||
| views_buf.push(view); |
There was a problem hiding this comment.
This can use Vec::collect
There was a problem hiding this comment.
views_buf is preallocated once, would Vec::collect allocate more buffer than currently?
| views_buf.push(view); | ||
| } | ||
|
|
||
| data_blocks.push(Buffer::from_vec(data_buf)); |
There was a problem hiding this comment.
This coul as well use collect.
4 participants
Which issue does this PR close?
Rationale for this change
Previously,
gc()will produce a single buffer. However, for buffer size greater than 2GiB, it would be buggy, since buffer-offset it's a 4-byte signed integer.What changes are included in this PR?
Add a GcCopyGroup type, and do gc for it.
Are these changes tested?
Yes
Are there any user-facing changes?
gc would produce more buffers